Skip to content

libc: retry open when interrupted#252

Merged
newpavlov merged 1 commit intorust-random:masterfrom
c3h2-ctf:open
Mar 25, 2022
Merged

libc: retry open when interrupted#252
newpavlov merged 1 commit intorust-random:masterfrom
c3h2-ctf:open

Conversation

@stoeckmann
Copy link
Copy Markdown
Contributor

The open call can be interrupted. Since sys_fill_exact covers this for
read operation already, it should be done here as well.

Signed-off-by: Tobias Stoeckmann tobias@stoeckmann.org

Shoutout to c3h2-ctf.

Copy link
Copy Markdown
Member

@josephlr josephlr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch here!!

Comment thread src/util_libc.rs
Comment on lines +110 to +114
loop {
let fd = open(path.as_ptr() as *const _, libc::O_RDONLY | libc::O_CLOEXEC);
if fd < 0 {
let err = last_os_error();
// We should try again if the call was interrupted.
if err.raw_os_error() != Some(libc::EINTR) {
return Err(err);
}
} else {
return Ok(fd);
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if it's more idiomatic to do:

let mut fd;
while { fd = open(...); fd < 0 } {
    let err = last_os_error();
    // We should try again if open() was interrupted.
    if err.raw_os_error() != Some(libc::EINTR) {
        return Err(err);
    }
}
Ok(fd)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively:

loop {
    let fd = open(path.as_ptr() as *const _, libc::O_RDONLY | libc::O_CLOEXEC);
    if fd >= 0 {
        return Ok(fd);
    }
    let err = las_os_error();
    // We should try again if open() was interrupted.
    if err.raw_os_error() != Some(libc::EINTR) {
        return Err(err);
    }
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have taken the second version, which received a thumbs up. :)

The open call can be interrupted. Since sys_fill_exact covers this for
read operation already, it should be done here as well.

Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>
@josephlr
Copy link
Copy Markdown
Member

I'll give @newpavlov a little time to take a look, and then I'll merge this.

Copy link
Copy Markdown
Member

@newpavlov newpavlov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it could be worth to limit number of retries to be extra safe by replacing loop { .. } with something like this:

for _ in 0..256 {
    // ..
}
Err(Error::TOO_MANY_ITERATIONS) // find a better name

@josephlr
Copy link
Copy Markdown
Member

I wonder if it could be worth to limit number of retries to be extra safe by replacing loop { .. } with something like this:

for _ in 0..256 {
    // ..
}
Err(Error::TOO_MANY_ITERATIONS) // find a better name

It looks like Rust's open implementation on Linux calls cvt_r and cvt_r just retries indefinitely. So we should probably mirror that behavior here.

@newpavlov newpavlov merged commit fcae1d2 into rust-random:master Mar 25, 2022
henrysun007 pushed a commit to mesatee/getrandom that referenced this pull request Nov 24, 2022
The open call can be interrupted. Since sys_fill_exact covers this for
read operation already, it should be done here as well.

Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>
takumi-earth pushed a commit to earthlings-dev/getrandom that referenced this pull request Jan 27, 2026
The open call can be interrupted. Since sys_fill_exact covers this for
read operation already, it should be done here as well.

Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants